Show cached fiat amounts immediately on transaction details #496
Conversation
- Add synchronous cached fiat methods to Rust TransactionDetails: amountFiatFmtCached, feeFiatFmtCached, sentSansFeeFiatFmtCached - Update iOS/Android AsyncView to accept optional cachedValue parameter - Display cached fiat values instantly, show spinner only when no cache - Both platforms now have consistent behavior for fiat amount loading
📝 WalkthroughWalkthroughThis PR introduces a caching mechanism for fiat amounts across Android, iOS, and Rust. New cached accessor methods are added to transaction details, AsyncView components are enhanced to accept optional cached values, and UI screens are updated to display cached values immediately while async operations fetch fresh data in the background. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
android/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt (3)
64-64: Consider keying LaunchedEffect to actual dependencies.Per the coding guidelines,
LaunchedEffectshould be keyed to actual dependencies rather thanUnit. Consider keying tooperationto re-run when the operation changes, or document whyUnitis the correct choice here (e.g., if the operation should only run once regardless of parameter changes).Trade-off: keying to
operationmakes the component responsive to changes but requires callers torememberthe lambda to avoid unnecessary re-runs.As per coding guidelines, LaunchedEffect should be keyed to dependencies.
91-92: Consider indicating when cached data is shown due to an error.When an error occurs and
cachedValueis provided, the component silently displays the cached value without indicating that:
- An error occurred while fetching fresh data
- The displayed data might be stale
Users may believe they're seeing fresh data when they're actually seeing cached fallback data. Consider adding a subtle visual indicator (e.g., a small warning icon or different styling) to communicate this state.
79-80: Optional: Consider adding subtle loading feedback when showing cached values.When
cachedValueis displayed during the loading state, there's no visual indication that fresh data is being fetched in the background. This is likely intentional to avoid UI flicker and provide immediate content, but users may not realize the data is being refreshed.If desired, consider adding a subtle loading indicator (e.g., a small spinner in a corner) to communicate that an update is in progress, while still showing the cached content prominently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsWidget.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SelectedWalletFlow/TransactionDetails/SentDetailsExpandedView.swiftios/Cove/Flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsView.swiftios/Cove/Views/AsyncView.swiftrust/src/transaction/transaction_details.rs
🧰 Additional context used
📓 Path-based instructions (3)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/transaction/transaction_details.rs
ios/Cove/**/*.swift
⚙️ CodeRabbit configuration file
ios/Cove/**/*.swift: - Review SwiftUI view code for proper layout, best practices
- Identify spelling mistakes in comments, string literals, and documentation
- Ignore generated bindings code in ios/CoveCore/Sources/CoveCore/generated/**
Files:
ios/Cove/Views/AsyncView.swiftios/Cove/Flows/SelectedWalletFlow/TransactionDetails/SentDetailsExpandedView.swiftios/Cove/Flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsView.swift
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt
⚙️ CodeRabbit configuration file
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt:⚠️ CRITICAL FFI/Threading Policy - READ FIRST:
NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))
Rust FFI calls are SYNCHRONOUS and FAST - they complete in microseconds
The Rust core uses Tokio runtime internally and handles all async operations
Database operations (redb) are optimized and do NOT block the UI thread
ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking
If you see a Rust FFI call on the main thread, DO NOT FLAG IT - this is correct
Ignore generated bindings code in android/app/src/main/java/org/bitcoinppl/cove_core/**
Verify correct functionality and logic consistency.
Check for idiomatic Kotlin usage and performance best practices.
FFI Resource Management:
- Always verify that UniFFI objects implementing AutoCloseable call .close() before being nulled
- Example: Mnemonic must call .close() to trigger zeroization of sensitive data
- Use DisposableEffect for cleanup in Compose, never just null references
Compose Best Practices:
- LaunchedEffect should be keyed to actual dependencies, not Unit
- Set loading states at the beginning of LaunchedEffect blocks
Files:
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsWidget.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt
🧬 Code graph analysis (4)
rust/src/transaction/transaction_details.rs (3)
rust/crates/cove-types/src/address.rs (1)
amount(227-229)rust/crates/cove-types/src/transaction/sent_and_received.rs (1)
amount(40-45)rust/crates/cove-types/src/psbt.rs (1)
fee(56-67)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt (1)
android/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt (1)
AsyncText(25-50)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsWidget.kt (1)
android/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt (1)
AsyncText(25-50)
ios/Cove/Flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsView.swift (2)
android/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt (1)
AsyncView(54-99)ios/CoveCore/Sources/CoveCore/generated/cove.swift (5)
transactionDetails(8403-8418)amountFiatFmtCached(9285-9291)amountFiatFmt(9268-9283)amount(7298-7304)amount(9243-9249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: compile-ios
- GitHub Check: compile-android
- GitHub Check: clippy
- GitHub Check: ktlint
🔇 Additional comments (21)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (5)
1628-1629: Auto-generated checksum declarations look correct.The new external checksum function declarations for the cached fiat formatting methods follow the established UniFFI pattern and are consistent with adjacent methods.
Also applies to: 1640-1641, 1652-1653
2686-2687: FFI function signatures are consistent.The cached methods correctly:
- Accept only the pointer parameter (no async continuation needed for synchronous cached access)
- Return
RustBuffer.ByValuefor nullable String values- Include the
uniffi_out_err: UniffiRustCallStatusparameter for error handlingAlso applies to: 2698-2699, 2710-2711
4123-4125: Checksum validations are correctly integrated.The checksum values (11182, 1845, 42399) are unique and properly placed adjacent to their non-cached counterparts, ensuring API compatibility verification at runtime.
Also applies to: 4141-4143, 4159-4161
20383-20384: Interface declarations correctly define nullable return types.The cached methods appropriately:
- Return
kotlin.String?(nullable) since cached values may not exist- Are non-suspending functions (synchronous access to cache)
- Follow the naming convention with
CachedsuffixAlso applies to: 20395-20396, 20407-20408
20597-20609: Implementations correctly use nullable string conversion.All three cached method implementations:
- Use
FfiConverterOptionalString.lift()for proper nullable String handling- Follow the
callWithHandle+uniffiRustCallpattern consistent with other methods- Call the correct FFI functions (
uniffi_cove_fn_method_transactiondetails_*_cached)This is auto-generated UniFFI code that should not be manually modified.
Also applies to: 20683-20695, 20769-20781
android/app/src/main/java/org/bitcoinppl/cove/views/AsyncView.kt (2)
55-60: LGTM! Good API design for progressive data loading.The addition of the optional
cachedValueparameter improves perceived performance by displaying cached data immediately while fresh data loads in the background. The defaultnullvalue maintains backward compatibility.
71-71: LGTM! Enhanced error logging.Including the exception throwable provides full stack traces, which significantly improves debugging and error tracking.
ios/Cove/Views/AsyncView.swift (1)
31-65: LGTM! Clean implementation of cached value support.The AsyncView now correctly displays cached values immediately during loading and on error, providing a better user experience by avoiding unnecessary loading spinners when data is available. The logic properly falls back to ProgressView/errorView when no cached value exists.
rust/src/transaction/transaction_details.rs (3)
233-238: LGTM! Clean cached accessor for amount fiat formatting.The method correctly retrieves the cached fiat value synchronously, avoiding the async overhead when cached data is available. The Option return type appropriately signals when no cached value exists.
261-266: LGTM! Consistent implementation with amount_fiat_fmt_cached.The fee caching follows the same pattern, properly handling the optional fee with early return.
305-310: LGTM! Completes the cached fiat formatting trio.The sent_sans_fee cached accessor maintains consistency with the other cached methods.
ios/Cove/Flows/SelectedWalletFlow/TransactionDetails/SentDetailsExpandedView.swift (3)
78-86: LGTM! Proper use of cached values for immediate display.The AsyncView now shows cached fee fiat values immediately while fetching fresh data in the background, improving perceived performance.
97-105: LGTM! Consistent caching pattern applied.The recipient receives section follows the same cached value pattern as the fee section.
119-127: LGTM! Complete implementation of cached fiat display.All three fiat amounts in the sent details view now use the cached value pattern consistently.
ios/Cove/Flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsView.swift (2)
107-112: LGTM! Cached fiat display for received transactions.The received transaction view now benefits from immediate cached fiat value display, consistent with the sent transaction implementation.
194-199: LGTM! Cached fiat display for sent transactions.The sent transaction view mirrors the received transaction implementation, maintaining consistency across both transaction types.
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsWidget.kt (2)
40-42: LGTM! Proper nullable types for cached fiat values.Making these parameters nullable correctly reflects that cached values may not be immediately available, aligning with the caching semantics introduced in this PR.
211-212: LGTM! AsyncText provides consistent loading behavior.The switch to AsyncText allows graceful handling of null cached values by showing a loading indicator, consistent with the iOS implementation. The added spacer maintains proper vertical spacing.
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt (3)
113-117: Excellent caching pattern for immediate display.The initialization with cached fiat accessors provides a great UX - values display immediately if cached (no spinner), while fresh values are fetched asynchronously in the background. When cached values are null, AsyncText appropriately shows a spinner.
132-155: Solid error handling that preserves cached values.The error handling pattern correctly keeps the cached value when async fetching fails, ensuring the UI always displays meaningful data. The LaunchedEffect is properly keyed to
transactionDetails, so it re-runs when the cache updates.Minor observation: The self-assignment pattern (
feeFiatFmt = feeFiatFmt) in the catch blocks is slightly unusual but works correctly to preserve the current value.
483-487: LGTM! AsyncText correctly implements the loading/display behavior.The AsyncText component appropriately handles both states: displaying the fiat amount when available and showing a spinner when null. This aligns perfectly with the cached-value initialization pattern, providing immediate display when cached values exist and smooth loading when they don't.
|
@greptile-apps review pr |
Greptile SummaryThis PR adds immediate display of cached fiat amounts to transaction details, eliminating loading delays when cache is available. Three new synchronous methods ( Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI (iOS/Android)
participant TD as TransactionDetails
participant FC as FiatClient
participant Cache as Cache (Memory/DB)
participant API as Mempool API
Note over UI,API: Initial Screen Load
UI->>TD: Request cached fiat amounts
TD->>FC: value_in_currency_cached()
FC->>Cache: Check memory cache
alt Cache Hit
Cache-->>FC: Return cached price
FC-->>TD: Calculate fiat value
TD-->>UI: Display cached fiat (immediate)
else Cache Miss
Cache-->>FC: None
FC-->>TD: None
TD-->>UI: null (show spinner)
end
Note over UI,API: Background Async Fetch
UI->>TD: Fetch fresh fiat amounts (async)
TD->>FC: current_value_in_currency()
FC->>API: GET /api/v1/prices
alt API Success
API-->>FC: Fresh prices
FC->>Cache: Update cache
FC-->>TD: Calculate fresh fiat value
TD-->>UI: Update display with fresh value
else API Error
API-->>FC: Error
FC-->>TD: Error
TD-->>UI: Keep showing cached value
end
|
- Add synchronous cached fiat methods to Rust TransactionDetails: amountFiatFmtCached, feeFiatFmtCached, sentSansFeeFiatFmtCached - Update iOS/Android AsyncView to accept optional cachedValue parameter - Display cached fiat values instantly, show spinner only when no cache - Both platforms now have consistent behavior for fiat amount loading
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.